[https://nvbugs/6506920][fix] Add stderr, stderr_margin_sigmas fields to HypothesisTestingParams…#16858
[https://nvbugs/6506920][fix] Add stderr, stderr_margin_sigmas fields to HypothesisTestingParams…#16858trtllm-agent wants to merge 1 commit into
stderr, stderr_margin_sigmas fields to HypothesisTestingParams…#16858Conversation
…ic stderr The DSpark GSM8K test's strict outer floor sat inside lm-eval's own reported per-metric stderr (~0.55 on 1319 samples), so natural evaluation noise straddled the 96.0 gate. Add optional 'stderr' and 'stderr_margin_sigmas' fields to HypothesisTestingParams and thread them from yaml entries in get_hypothesis_testing_params. When set, ref_accuracy is treated as an anchor and adjusted by stderr_margin_sigmas * stderr for strict-outer comparisons; the original is retained as ref_accuracy_anchor and surfaced in the hypothesis-testing report. The DSpark GSM8K reference is updated to record the true anchor (96.475) and lm-eval's own reported stderr (0.55); the default 2-sigma margin yields an effective floor of 95.375, above the observed spread. Backward-compatible: entries without a stderr field are unchanged. Remove the DSpark test from waives.txt now that the strict floor is stderr-aware. Verified: score 96.058 >= adjusted ref 95.375 -> PASS. Signed-off-by: handongl <handongl@nvidia.com>
WalkthroughChangesAccuracy stderr handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ReferenceConfig
participant AccuracyTask
participant HypothesisTestingParams
participant AccuracyReport
ReferenceConfig->>AccuracyTask: provide entry stderr
AccuracyTask->>HypothesisTestingParams: pass stderr
HypothesisTestingParams->>HypothesisTestingParams: anchor and adjust ref_accuracy
HypothesisTestingParams->>AccuracyReport: include anchor and adjustment
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/integration/defs/accuracy/accuracy_core.py (1)
84-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest coverage summary — needs follow-up.
- Test functions added, modified, or removed: none in the provided diff.
- Test-list membership: not applicable to changed test functions; no test-list entry is shown.
- Coverage verdict: needs follow-up. Add focused tests for higher- and lower-is-better metrics, zero/invalid stderr, configurable sigma margins, and report output. Integration validation requires GPU/model access with
LLM_MODELS_ROOTset.As per path instructions, changed test infrastructure requires an explicit coverage assessment.
Also applies to: 204-213
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/defs/accuracy/accuracy_core.py` around lines 84 - 126, Add focused tests covering __post_init__ for higher- and lower-is-better metrics, including zero, invalid, and configured stderr margin values, and verify that ref_accuracy and threshold are adjusted correctly. Add report tests validating anchor and adjustment output when stderr is positive and unchanged output otherwise. Run the integration validation with GPU/model access and LLM_MODELS_ROOT configured.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/integration/defs/accuracy/accuracy_core.py`:
- Line 72: Make stderr_margin_sigmas configurable in the HypothesisTestingParams
API instead of relying only on STDERR_MARGIN_SIGMAS. Update the relevant
parameter construction and reference-entry handling, including
get_hypothesis_testing_params(), to forward entry.get("stderr_margin_sigmas",
STDERR_MARGIN_SIGMAS) so entries can override the default while preserving 2.0
when unspecified.
---
Nitpick comments:
In `@tests/integration/defs/accuracy/accuracy_core.py`:
- Around line 84-126: Add focused tests covering __post_init__ for higher- and
lower-is-better metrics, including zero, invalid, and configured stderr margin
values, and verify that ref_accuracy and threshold are adjusted correctly. Add
report tests validating anchor and adjustment output when stderr is positive and
unchanged output otherwise. Run the integration validation with GPU/model access
and LLM_MODELS_ROOT configured.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a65bcf1e-d9f5-4950-814a-52357f15d345
📒 Files selected for processing (3)
tests/integration/defs/accuracy/accuracy_core.pytests/integration/defs/accuracy/references/gsm8k.yamltests/integration/test_lists/waives.txt
💤 Files with no reviewable changes (1)
- tests/integration/test_lists/waives.txt
| return ref_accuracy - z_alpha * scale | ||
|
|
||
|
|
||
| STDERR_MARGIN_SIGMAS = 2.0 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Implement the advertised stderr_margin_sigmas parameter.
The PR contract describes stderr_margin_sigmas as configurable, but this class only exposes the hard-coded STDERR_MARGIN_SIGMAS = 2.0; reference entries cannot select another margin.
Suggested fix
class HypothesisTestingParams:
stderr: Optional[float] = None
+ stderr_margin_sigmas: float = STDERR_MARGIN_SIGMAS
def __post_init__(self) -> None:
self.ref_accuracy_anchor = self.ref_accuracy
if self.stderr is not None and self.stderr > 0:
sign = -1 if self.higher_is_better else 1
- self.ref_accuracy += sign * STDERR_MARGIN_SIGMAS * self.stderr
+ self.ref_accuracy += sign * self.stderr_margin_sigmas * self.stderrAlso forward entry.get("stderr_margin_sigmas", STDERR_MARGIN_SIGMAS) from get_hypothesis_testing_params().
Based on the PR objectives, stderr_margin_sigmas is part of the intended HypothesisTestingParams API.
Also applies to: 84-96, 204-213
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/integration/defs/accuracy/accuracy_core.py` at line 72, Make
stderr_margin_sigmas configurable in the HypothesisTestingParams API instead of
relying only on STDERR_MARGIN_SIGMAS. Update the relevant parameter construction
and reference-entry handling, including get_hypothesis_testing_params(), to
forward entry.get("stderr_margin_sigmas", STDERR_MARGIN_SIGMAS) so entries can
override the default while preserving 2.0 when unspecified.
Summary
stderr,stderr_margin_sigmasfields to HypothesisTestingParams (defaults keep old behavior); in post_init, when stderr is set, save the original as ref_accuracy_anchor and subtract 2σ from ref_accuracy so the strict outer read is stderr-aware. Thread from yaml viaentry.get("stderr"). Record anchor 96.475 and stderr 0.55 in gsm8k.yaml's DSpark entry (effective floor 95.375). Remove the DGX_B200 DSpark waiver.Test plan
Links
Dev Engineer Review
96.475andstderr: 0.55, yielding an effective threshold of95.375.QA Engineer Review